Cookie Authentication #896
Conversation
Orso.Arpa.Api/Startup.cs
Outdated
| options.DefaultChallengeScheme = IdentityConstants.ApplicationScheme; | ||
| options.DefaultSignInScheme = IdentityConstants.ExternalScheme; | ||
| }) | ||
| .AddIdentityCookies(); |
There was a problem hiding this comment.
configuring authentication scheme and cookies is necessary since using AddIdentityCore requires more direct control to the setup
If we were using AddIdentity then authentication schemes and cookies would be configured automatically without a need for these initialisations
|
|
||
| if (_identityConfiguration.UseCookies) | ||
| { | ||
| result = await _signInManager.PasswordSignInAsync(user, request.Password, false, true); |
There was a problem hiding this comment.
signInManager.PasswordSignInAsync() can only be used with cookies
.gitignore
Outdated
| .dccache | ||
| Orso.Arpa.Api/__azurite_db_blob__.json |
There was a problem hiding this comment.
I think we can get that out of here. You can configure where azurite stores the data and this should not necessarily be in the project folder.
| @@ -30,13 +30,15 @@ public AuthController(IAuthService authService) | |||
| /// <response code="422">If validation fails</response> | |||
| [AllowAnonymous] | |||
| [HttpPost("login")] | |||
| [ProducesResponseType(StatusCodes.Status204NoContent)] | |||
| [ProducesResponseType(StatusCodes.Status200OK)] | |||
There was a problem hiding this comment.
If the endpoint returns No Content, we should remove the Status200OK attribute
| @@ -206,6 +209,7 @@ public async Task<ActionResult<TokenDto>> RefreshAccessToken() | |||
| public async Task<ActionResult> Logout() | |||
| { | |||
| await _authService.RevokeRefreshTokenAsync(RefreshToken, RemoteIpAddress); | |||
| await _authService.SignOut(); | |||
There was a problem hiding this comment.
the controller should be as dump as possible and only call one single service method. The service then acts as an orchestrator and can send multiple commands or queries, if required
| using Microsoft.AspNetCore.Http; | ||
| using Microsoft.AspNetCore.Mvc; | ||
|
|
||
| namespace Orso.Arpa.Api.Extensions |
There was a problem hiding this comment.
this is not an extension class, is it? Then it should be moved to another appropriate place
Orso.Arpa.Api/Startup.cs
Outdated
| @@ -452,6 +465,27 @@ private void ConfigureAuthentication(IServiceCollection services) | |||
| .AddRoleManager<RoleManager<Role>>() | |||
| .AddUserManager<ArpaUserManager>(); | |||
|
|
|||
| JwtConfiguration jwtConfig = AddConfiguration<JwtConfiguration>(services); | |||
|
|
|||
| _ = identityBuilder.Services.AddAuthentication(CookieAuthenticationDefaults.AuthenticationScheme) | |||
There was a problem hiding this comment.
I think "RegisterServices" is not the appropriate method for this configuration.
Plus: We now have "services.AddAuthentication" twice in the Startup class. There should be only one authentication configuration
| @@ -6,6 +7,8 @@ namespace Orso.Arpa.Domain.General.Interfaces | |||
| { | |||
| public interface IJwtGenerator | |||
There was a problem hiding this comment.
As this one is not used for jwt anymore it should get a new and more appropriate name
There was a problem hiding this comment.
TokenGenerator? AuthenticationHelper?
There was a problem hiding this comment.
Yes for example. I think the name of the second method is misleading, as it does not Get the claims identity from a user object, but it creates a claims identity object with data from the user object. Maybe you find a better name for it?
| @@ -794,7 +794,7 @@ protected override void BuildTargetModel(ModelBuilder modelBuilder) | |||
| CreatedAt = new DateTime(2021, 6, 16, 15, 30, 19, 324, DateTimeKind.Local).AddTicks(7866), | |||
| CreatedBy = "LocalizationSeedData", | |||
| Deleted = false, | |||
| Key = "Invalid token supplied", | |||
There was a problem hiding this comment.
Migration files are automatically generated by entity framework and must not be updated manually - they are already executed on the prod database!
Please update the files in the "Translation" folder and execute a new migration instead (see how-to in readme file of the persistence project)
| @@ -1,7 +1,7 @@ | |||
| [ | |||
| { | |||
| "Key": "This request requires a valid JWT access token to be provided", | |||
There was a problem hiding this comment.
the files in the Localization folder are generated automatically during migration and must not be updated manually (see readme file in this folder)
|
this pr is to make the changes gradually and under control, to prevent giant modifications at once. also some comments will be added for documentation purposes.